-
Notifications
You must be signed in to change notification settings - Fork 554
Pyomo. DoE: Sensitivity initialization #3639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…pected. Now, I need to add cases when some of the design variables are not changed.
…mo into sensitivity-initialization
… use compute_FIM_metrics() method. Added both the nested for loop and change_one_design_at_a_time argument to compute_FIM_factorial() in doe.py. Everything is working fine.
…gn_values` does not need to be in the same order as the `experiment_inputs` as long as the key matches, we are good. Also, if we don't want to change a design variable, just not passing it as a key in the `design_values` dictionary will do the trick. Tested with the `reactor_example.py`
@adowling2 @djlaky, this draft PR is ready for initial review. |
Does this PR depend on changes in #3575? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smondal13 I left some initial comments.
pyomo/contrib/doe/utils_updated.py
Outdated
@@ -0,0 +1,267 @@ | |||
# ___________________________________________________________________________ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smondal13 Why are your creating a new utils
file instead of updating the current one? There might be a point of confusion regarding branches in git.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue is resolved now. merged the utils file from my previous PR # 3525.
@@ -1617,6 +1628,221 @@ def compute_FIM_full_factorial( | |||
|
|||
return self.fim_factorial_results | |||
|
|||
def compute_FIM_factorial( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this function already in Pyomo.DoE somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I created this method. I do not see the same name anywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adowling2, we have compute_FIM_full_factorial()
. I have not changed that method, rather I have added a new method. Maybe we can show a deprecation warning for compute_FIM_full_factorial()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a depreciation warning sounds reasonable.
pyomo/contrib/doe/doe.py
Outdated
# design_map_keys so that design_point can be constructed correctly in the loop. | ||
des_ranges = [design_values[k.name] for k in design_map_keys] | ||
if change_one_design_at_a_time: | ||
factorial_points = generate_snake_zigzag_pattern(*des_ranges) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's brainstorm different sensitivity analysis sequences we might use. We can then define an enum.
pyomo/contrib/doe/utils.py
Outdated
@@ -100,3 +100,43 @@ def rescale_FIM(FIM, param_vals): | |||
# pass # ToDo: Write error for suffix keys that aren't ParamData or VarData | |||
# | |||
# return param_list | |||
|
|||
|
|||
def generate_snake_zigzag_pattern(*lists): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's come up with a better name. I do not think of "snake" as a technical term.
yield (value,) + sub_pattern | ||
|
||
# Start the recursion at the first list (depth 0) with an initial sum of 0. | ||
yield from _generate_recursive(depth=0, index_sum=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are we going to test this function? IMO, the next step is to develop some tests for this. That way, as you try to run the sensitivity analysis for an example, you are least know this part of the code is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adowling2 , I did check it manually before. I have recently added a test for this in test_utils.py
for up to 3 variables, and also for different array-like structures. From the test, it works the way we want.
Also added test for patter generator
…mo into sensitivity-initialization
…ensitivity-initialization
…f. The code is run by using a simple model.
@adowling2 , @djlaky, @blnicho This PR is ready for design review. |
@@ -1617,6 +1628,221 @@ def compute_FIM_full_factorial( | |||
|
|||
return self.fim_factorial_results | |||
|
|||
def compute_FIM_factorial( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a depreciation warning sounds reasonable.
file_name: str = None, | ||
): | ||
"""Will run a simulation-based factorial exploration of the experimental input | ||
space (i.e., a ``grid search`` or ``parameter sweep``) to understand how the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "i.e." should this be "a.k.a." (also knowns as)
factorial_points = snake_traversal_grid_sampling(*design_values) | ||
elif scheme_enum == SensitivityInitialization.nested_for_loop: | ||
factorial_points = product(*design_values) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to split the function here? That way, we can do extensive testing on the results of factorial_points
without running the rest of the function.
---------- | ||
model : DoE model, optional | ||
The model to perform the full factorial exploration on. Default: None | ||
design_vals : dict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading this doc string, I do not fully appreciate all of options. I do not think we are looking at all of the edge cases with the tests.
@smondal13 As a next step, I suggest you create a Google Doc as a mock-up for the documentation you plan to write for this new feature. In the mock-up, you can provide example usage to show the programmatic interface. I would set the Google Doc to allow for anonymous commenting and post the link here. Alternatively, you could do this in a markdown |
Fixes # .
Summary/Motivation:
Changes proposed in this PR:
compute_FIM_factorial()
in Pyomo.DoE for sensitivity initializationsnake_traversal_grid_sampling()
andcompute_correlation_matrix()
in utils.pyLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: